Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support FailureDiffs without configuration #71

Closed
wants to merge 2 commits into from

Conversation

hugobast
Copy link

@hugobast hugobast commented Nov 30, 2016

This took an unexpected turn. There's a nice interface defined for the
reference folder:
https://github.com/facebook/ios-snapshot-test-case/blob/master/FBSnapshotTestCase/FBSnapshotTestController.h#L72

That's not the case for the failure diffs folder. I had to get creative
so I used the setenv in order to give this failure diffs folder mapping
by default. See here to know how it's used in the FBSnapshotTestCase
project:
https://github.com/facebook/ios-snapshot-test-case/blob/master/FBSnapshotTestCase/FBSnapshotTestController.m#L266-L268

With all that said I'm still opening a PR cause I said I would and it was pretty straightforward but I won't feel bad if this gets rejected given the implementation I am submitting. I could take a further step and propose a change in FBSnapshotTestCase but I am not sure if I want to give that a shot.

Ref: #68

```
# Nimble-Snapshot
FailureDiffs
```
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, good job on the documentation 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this should be even more in evidence? I'd put it as a subitem of Use, before Dynamic Type

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll move it up!

@hugobast
Copy link
Author

Ok so I have the same test failures locally, I let Travis confirmed that it wasn't simply a problem with my setup, like I didn't have my iPhone6 simulator running on 10.0 but 10.1 instead and that caused some of the output files to have the wrong path. I'll look into those failures.

@ashfurrow
Copy link
Owner

It's possible there are changes between 10.0 and 10.1 that caused the snapshot reference images to change. What do the diffs look like? We might just need to replace them with new ones.

@hugobast
Copy link
Author

hugobast commented Dec 2, 2016

I haven't forgotten; I'm having a look tomorrow since it's Friday and I should be able to find the time 👍

@orta
Copy link
Collaborator

orta commented May 15, 2017

Poke ^ :D

@hugobast
Copy link
Author

Thanks for poking, I'll try to get this week

@ashfurrow
Copy link
Owner

Do your best, no pressure! And let us know if you need anything from our side.

@hugobast
Copy link
Author

hugobast commented Jun 1, 2017

I'll reopen a new PR for this, I rebased on upstream but something got screwed up, and I know why now.

@hugobast hugobast closed this Jun 1, 2017
@hugobast hugobast reopened this Jun 1, 2017
This took an unexpected turn. There's a nice interface defined for the
reference folder:
https://github.com/facebook/ios-snapshot-test-case/blob/master/FBSnapshotTestCase/FBSnapshotTestController.h#L72

That's not the case for the failure diffs folder. I had to get creative
so I used the setenv in order to give this failure diffs folder mapping
by default. See here to know how it's used in the FBSnapshotTestCase
project:
https://github.com/facebook/ios-snapshot-test-case/blob/master/FBSnapshotTestCase/FBSnapshotTestController.m#L266-L268
@hugobast
Copy link
Author

hugobast commented Jun 1, 2017

All green now @ashfurrow

@marcelofabri
Copy link
Collaborator

I think something is not quite right: I've made the WithoutQuickTests test to fail, but the FailureDiff folder was created inside of ACustomFolder (and not ReferenceImages as I'd expect).

@hugobast
Copy link
Author

hugobast commented Jun 1, 2017

Ok @marcelofabri I'll see why and fix it.

@hugobast hugobast closed this Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants